Skip to content

Conversation

@tlconnor
Copy link

@tlconnor tlconnor commented Sep 14, 2016

When assert_predicate assertions fail Minitest will inspect the target object as part of the error message generation.

Currently AePageObjects will pass the inspect call on to the Element which will attempt to load the DOM element and will result in AePageObjects::LoadingElementFailed being thrown.

This PR allow inspect to be called on ElementProxy objects where the DOM element is absent without throwing an error.


implicit_element.__send__(name, *args, &block)
rescue AePageObjects::LoadingElementFailed
raise unless %w(to_s inspect).include? name.to_sym
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included both to_s and inspect here because I figured that both methods would be better off returning a string rather than raising an error. I could be convinced otherwise.

@rmacklin
Copy link
Contributor

Can you add tests? I tried to reproduce this in an existing selenium test for a simple project and wasn't been able to, so I think there is more to it than what was described above.

@tlconnor tlconnor force-pushed the allowMissingElementsToBeInspected branch from 89a55e4 to eb7072f Compare September 14, 2016 17:14
@tlconnor
Copy link
Author

tlconnor commented Sep 14, 2016

@rmacklin you are right, the root cause of the flakiness was something else. The fix I've made is still valid as the failing inspect call would hide the real error.

I've added unit tests for the fix.

@rmacklin
Copy link
Contributor

rmacklin commented Sep 14, 2016

I'm still not totally clear on the root issue here. But if this is limited to Minitest::Assertions#assert_predicate assertions, did you see this comment in the file you linked in the original PR description (before you edited it)? Seems like overriding that method in a minitest extensions module would be smaller in scope than globally changing how inspect and to_s behave, which is technically a breaking change.

@dtognazzini
Copy link
Contributor

Seems like a pretty good improvement. I think things would become even cleaner if we collapsed ElementProxy into Element altogether: #127 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants